Skip to content

fix(cubemaster): support never-timeout semantics for refresh and update - #862

Open
emailcannotbeblank wants to merge 7 commits into
TencentCloud:masterfrom
emailcannotbeblank:fix/add-missing-timeout-field
Open

fix(cubemaster): support never-timeout semantics for refresh and update#862
emailcannotbeblank wants to merge 7 commits into
TencentCloud:masterfrom
emailcannotbeblank:fix/add-missing-timeout-field

Conversation

@emailcannotbeblank

@emailcannotbeblank emailcannotbeblank commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

1. Never-timeout support and lifecycle timeout alignment

1.1 Problem

UpdateRequest in CubeMaster had no timeout field, so the timeout sent by CubeAPI was not carried through on resume. Beyond that, refresh and resume did not fully support never-timeout: refresh rejected -1, and resume silently coerced any negative value to -1.

1.2 Before (master)

API < -1 -1 0 omitted positive N
create normalized to -1 (resolveTimeoutSeconds) never-timeout immediate timeout cluster default (default -1 → never) N seconds
set_timeout 400 never-timeout (endAt = now-1s, bogus) immediate timeout required N seconds, no upper bound
refresh error after pass-through (duration must be positive) error (never-timeout unsupported) error (duration must be positive) error (CubeAPI unwrap_or(0)must be positive) N seconds, no upper bound (validator annotation never executed)
resume / connect(paused) resume OK, timeout not carried through (UpdateRequest has no field) same same same same
connect(running) connection info returned, timeout ignored (no validation annotations, handler never validates) same same same same

1.3 After (this PR; bold = changed)

API < -1 -1 0 omitted positive N
create normalized to -1 (unchanged) never-timeout immediate timeout cluster default N seconds
set_timeout 400 never-timeout (endAt now 0, the canonical no-deadline value; was now − 1s, a timestamp already in the past) immediate timeout required N seconds, no upper bound
refresh 400 (validated on both CubeAPI and CubeMaster; was a pass-through error) never-timeout (was error) 400 (rejection reason changed — immediate timeout belongs to set_timeout(0) — result unchanged) cluster default (resolveTimeoutSeconds(nil, DefaultTimeoutInsec); was error) N seconds, no upper bound
resume / connect(paused) 400 (validate_timeout_value; was silently dropped) resume + never-timeout (was dropped) resume + preserve stored timeout, rebase idle window (RebaseTimeoutWindow; was dropped) same as 0 (was dropped) resume + N seconds (was dropped)
connect(running) 400 (validation annotations + body.validate() added to ConnectSandbox; was silently ignored) connection info, timeout unchanged same same same

1.4 Why these changes

Per the maintainer: resume reject invalid values (< -1) with an explicit error rather than silently normalizing them, and that it share the same value model as set_timeout and the other APIs (-1 = never-timeout, 0 = immediate timeout, supported by set_timeout only, positive = TTL); an omitted refresh duration must not become an immediate timeout and should fall back to the cluster default.


2. E2E tests

All cases run against real sandboxes via the shared adapter contract in tests/e2e/sdk_compat/cases/lifecycle/ (test_refresh.py, test_timeout.py, test_pause_resume.py). SDK-backed cases are collected for both backends (CubeSandbox SDK and E2B SDK); refresh cases drive the CubeAPI REST endpoint directly. Success cases additionally verify the sandbox stays usable (run_command).

Area Backend / entry Input Expected result
refresh CubeAPI REST omitted duration success; sandbox remains usable
refresh CubeAPI REST duration=-1 success; never-timeout
refresh CubeAPI REST duration=60 success; positive TTL
refresh CubeAPI REST duration=7200 success; positive TTL beyond 3600 (no upper bound)
refresh CubeAPI REST duration=0 400, error mentions duration
refresh CubeAPI REST duration=-2 400, error mentions duration
set_timeout CubeSandbox SDK + E2B SDK timeout=-1 success; never-timeout; sandbox remains usable
set_timeout CubeSandbox SDK + E2B SDK timeout=60 success; positive TTL; sandbox remains usable
set_timeout CubeSandbox SDK + E2B SDK timeout=0 success; immediate timeout is accepted
set_timeout CubeSandbox SDK + E2B SDK timeout=-2 rejected (HTTP 400); sandbox remains usable
resume CubeSandbox SDK + E2B SDK timeout=-1 success; never-timeout; sandbox resumes and remains usable
resume CubeSandbox SDK + E2B SDK timeout=0 success; preserves stored timeout by design; sandbox resumes and remains usable
resume CubeSandbox SDK + E2B SDK timeout=60 success; positive TTL; sandbox resumes and remains usable
resume CubeSandbox SDK + E2B SDK timeout=-2 rejected (HTTP 400, error mentions timeout)
pause/resume round-trip CubeSandbox SDK + E2B SDK files / env vars / run-code state preserved across pause → resume

Run with:

pytest --run-e2e --sdk-e2e-backends=cubesandbox,e2b -q \
  tests/e2e/sdk_compat/cases/lifecycle/{test_refresh,test_timeout,test_pause_resume}.py

3. E2B SDK endAt compatibility

3.1 Problem

CubeSandbox represents a never-timeout sandbox internally as end_at = None, and the field is omitted from JSON responses (skip_serializing_if = "Option::is_none"). The E2B SDK's SandboxDetail / ListedSandbox models however treat endAt as a required valid datetime — a missing or null endAt fails deserialization, so get_info() / list() broke for never-timeout sandboxes.

3.2 Design

Compatibility is applied at the CubeAPI handler boundary, on the read paths GET /sandboxes/{id}, GET /sandboxes, and GET /v2/sandboxes:

  1. Detect E2B SDK requests by User-Agent prefix: e2b-python-sdk/, e2b-js-sdk/, e2b-code-interpreter/.
  2. Fill the sentinel only for confirmed never-timeout sandboxes, i.e. all three of: the request is from an E2B SDK (UA match); timeout_seconds == -1 in the lifecycle metadata; end_at is None.
  3. Sentinel value: 9999-12-31T23:59:59Z, built at compile time from Unix seconds 253_402_300_799 (no per-request parsing).
  4. Cube-native callers are unaffected: without an E2B UA the field stays omitted, preserving the existing contract.

The timeout_seconds == -1 condition is load-bearing: an omitted end_at without timeout_seconds = -1 means the metadata did not resolve a timeout, and such a sandbox must not be presented as never-timeout. This distinguishes "confirmed no deadline" from "unknown".

Consistency: never-timeout is stored as TimeoutSeconds=-1, EndAt=0; CubeAPI reads end_at<=0 as None and omits it from responses; only E2B SDK requests that meet the three conditions get the sentinel. There is exactly one representation path for "no deadline" across the stack, so the values seen at each layer never disagree.

Error handling: sandboxes whose metadata has no resolved timeout (e.g. legacy sandboxes, timeout_seconds empty) fail the three-condition check, so their endAt stays omitted and they are never misreported as never-timeout.

3.3 Coverage

  • Rust unit tests: detail/list compat fills the sentinel only under the three conditions; UA matching matrix (including compound agents like e2b-js-sdk/2.28.0 e2b-cli/2.0.0, and empty headers); native responses keep endAt omitted.
  • E2E (test_timeout.py): after set_timeout(-1), all three E2B UA markers receive the sentinel on both detail and list endpoints, and a Cube-native UA sees endAt omitted — verified on both SDK backends.

Assisted-by: codex:gpt5.6-sol

@emailcannotbeblank

Copy link
Copy Markdown
Contributor Author

Note About E2B Refresh Semantics

In E2B, refresh-sandbox can only extend the sandbox lifetime; it does not shorten it.

The E2B refresh handler calls:

a.orchestrator.KeepAliveFor(ctx, teamID, sandboxID, duration, false)

The last argument is allowShorter=false. Inside KeepAliveFor, E2B skips the update when the newly calculated endTime is earlier than the current sbx.EndTime:

if !allowShorter && endTime.Before(sbx.EndTime) {
    return sbx, nil
}

By contrast, CubeSandbox refresh currently rewrites CreatedAt, TimeoutSeconds, and EndAt directly. This means CubeSandbox refresh may shorten the sandbox lifetime if the new duration is smaller than the currently remaining lifetime.

Comment thread CubeMaster/pkg/service/sandbox/types/types.go Outdated
Comment thread CubeAPI/src/models/mod.rs Outdated
Comment thread CubeMaster/pkg/service/sandbox/sandbox_timeout_test.go Outdated

@chenhengqi chenhengqi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am fine with the changes. @fslongjin Please double check.

@chenhengqi chenhengqi moved this from Todo to In progress in CubeSandbox Jul 14, 2026
@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from b07f7f0 to 6618a6c Compare July 14, 2026 16:36
if req.Action == "resume" && req.Timeout != nil && *req.Timeout < 0 {
timeout := types.NeverTimeout
req.Timeout = &timeout
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Silent normalization of invalid negative values + request struct mutation

This silently normalizes any value < 0 (e.g. -2, -100, -999) to NeverTimeout without any indication to the caller. On SetTimeout and Refresh, the same invalid values are explicitly rejected with a clear error. This asymmetry is a maintainability trap — developers working across both paths will find the behavior surprising.

Additionally, the mutation of req.Timeout happens before the cubelet RPC (line 80). If that RPC fails, the normalized value (-1) persists in the caller's request struct, so a retry with the same struct would silently change semantics. Consider either:

  1. Rejecting values < -1 here too (consistent with SetTimeout/Refresh), or
  2. At minimum, logging the normalization, and using a local variable instead of mutating req.Timeout.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed per option 1: Update now explicitly rejects values < -1 (consistent with SetTimeout/Refresh) and no longer mutates req.Timeout — a test verifies that -2 returns the error with the struct left untouched (sandbox_update_test.go:29-33).

Comment thread CubeMaster/pkg/service/sandbox/sandbox_timeout.go
@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch 3 times, most recently from 719864d to 380b561 Compare July 20, 2026 17:31
@emailcannotbeblank

Copy link
Copy Markdown
Contributor Author

Hi maintainers, friendly ping.

This PR has been rebased onto the latest master, with conflicts resolved and commits cleaned up.

The remaining Claude Auto Review failure is caused by the pull_request_target workflow rejecting fork PR checkout, rather than this PR’s code.
@fslongjin

@fslongjin fslongjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this PR! The core fix is solid — adding the missing timeout field to UpdateRequest and fixing the refresh validation gap are both real bugs that needed fixing.

I did a thorough review across several dimensions (security, correctness, architecture, test coverage, docs). The functional logic is correct, but I found a few things worth addressing before merging:

Blocking items

  1. Missing input validation on resume_sandboxResumedSandbox has no Validate derive and the handler never calls body.validate(), unlike set_sandbox_timeout and refresh_sandbox which both validate. This is a defense-in-depth gap (see inline comments).

  2. Inconsistent timeout validation strategySetTimeout and Refresh reject timeout < -1 with a clear error, but Update/resume silently coerces any negative value to -1. A caller sending timeout=-2 gets an explicit error from SetTimeout but a 200 from resume. This is a behavioral trap for SDK authors (see inline comment on sandbox_update.go:53).

  3. Missing upper-bound validation in CubeMaster's Refresh() — CubeAPI enforces max=3600 on refresh duration, but the Go side only checks Duration < -1 with no upper bound (see inline comment on sandbox_timeout.go:98).

Non-blocking suggestions

  • The default duration=0 (immediate timeout) when no duration is provided in refresh is a destructive default — worth documenting explicitly or reconsidering.
  • New function publishUpdateTimeout and field UpdateRequest.Timeout are missing doc comments.
  • The refreshTimeoutMeta fallback for -1 timeout computes time.Now().UnixMilli() + (-1)*1000 which returns a ~1-second-past timestamp as EndAt. The lifecycle sweeper isn't affected (it reads from Redis directly), but API consumers receiving a past timestamp for a never-timeout sandbox might be confused. Consider adding a guard:
    if timeoutSeconds < 0 {
        return 0 // never-timeout: sentinel for "no expiry deadline"
    }
    return time.Now().UnixMilli() + int64(timeoutSeconds)*1000

All three blocking items are small-scope fixes (~15 lines total across Rust and Go). Once addressed, this PR looks good to merge.

Path(sandbox_id): Path<String>,
Json(body): Json<RefreshRequest>,
) -> AppResult<impl IntoResponse> {
body.validate()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 Good catch adding body.validate() here — the RefreshRequest struct had validation annotations that were never being invoked before this PR.

However, the same gap also exists in resume_sandbox (line 289). The ResumedSandbox struct at models/mod.rs:324 has neither a Validate derive nor validation annotations on its timeout field, and the handler never calls body.validate() either.

This means an attacker or buggy client can currently send arbitrary i32 values (e.g., i32::MIN, i32::MAX) as a resume timeout, bypassing the CubeAPI validation layer entirely. CubeMaster does normalize negative values to -1 as defense-in-depth, but this validation gap should be closed at the API boundary for consistency with the other two timeout endpoints.

Suggested fix:

In models/mod.rs, update ResumedSandbox:

#[derive(Debug, Deserialize, Validate, ToSchema)]  // add Validate
#[allow(dead_code)]
pub struct ResumedSandbox {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    #[validate(custom(function = "validate_timeout_value"))]  // add this
    pub timeout: Option<i32>,
    #[serde(rename = "autoPause", default)]
    pub auto_pause: bool,
}

Then in the handler, add body.validate().map_err(|e| AppError::BadRequest(e.to_string()))?; after the destructuring, matching the pattern used here at line 470.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed as suggested above.

Comment thread CubeAPI/src/models/mod.rs Outdated
#[derive(Debug, Deserialize, Validate, ToSchema)]
pub struct RefreshRequest {
#[validate(range(min = 0, max = 3600))]
#[validate(range(min = -1, max = 3600))]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 The range change from min = 0 to min = -1 is the right fix — refresh should accept the same -1 never-timeout sentinel that SetTimeoutRequest already does.

Side note: while we're fixing validation coverage on the Rust side for this PR, the same gap exists in the ResumedSandbox struct (line 324). It has no #[derive(Validate)] and its timeout field has no #[validate(...)] annotation, so resume requests bypass CubeAPI-level timeout validation. Would be great to fix it in the same PR for consistency — validate_timeout_value (line 495) already exists and can be reused.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed. ResumedSandbox now derives Validate, timeout reuses validate_timeout_value, and resume_sandbox calls body.validate() before forwarding the request.

rsp.Ret = ret
return
}
if req.Action == "resume" && req.Timeout != nil && *req.Timeout < 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue: The validation strategy here is inconsistent with SetTimeout and Refresh.

SetTimeout (sandbox_timeout.go:49) and Refresh (sandbox_timeout.go:98) both reject values < -1 with ErrorCode_MasterParamsError:

if req.Timeout < -1 {
    rsp.Ret.RetCode = int(errorcode.ErrorCode_MasterParamsError)
    rsp.Ret.RetMsg = "timeout must be >= -1 (use -1 for never timeout)"
    return
}

But this resume path silently coerces any negative value to -1:

if req.Action == "resume" && req.Timeout != nil && *req.Timeout < 0 {
    timeout := types.NeverTimeout
    req.Timeout = &timeout
}

A caller sending timeout=-2 to SetTimeout gets a clear error. The same caller sending timeout=-2 to resume gets a 200 with the value silently changed to never-timeout. This is a behavioral trap.

Suggested fix: align with SetTimeout/Refresh:

if req.Action == "resume" && req.Timeout != nil && *req.Timeout < -1 {
    rsp.Ret.RetCode = int(errorcode.ErrorCode_MasterParamsError)
    rsp.Ret.RetMsg = "timeout must be >= -1 (use -1 for never timeout)"
    return
}

Bonus: the current < 0 condition also captures -1 (the valid NeverTimeout sentinel) and does a no-op -1 → -1 assignment, which is correct but confusing to read. Changing to < -1 fixes that readability issue too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Update/resume now rejects explicit timeout values < -1 with ErrorCode_MasterParamsError, matching SetTimeout and Refresh.

In the previous version, I noticed that sandbox creation normalized timeout values < -1 to -1, so the earlier change intentionally followed that normalization behavior for consistency with create. After your feedback, I aligned resume with the stricter validation strategy used by SetTimeout and Refresh instead.

return
}
if req.Duration <= 0 {
if req.Duration < -1 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

issue: This check only validates the lower bound (Duration < -1), with no corresponding upper-bound check.

CubeAPI's RefreshRequest enforces #[validate(range(min = -1, max = 3600))] (models/mod.rs:506), capping the duration at 3600 seconds. But here in CubeMaster there's no equivalent — if someone calls CubeMaster directly (or CubeAPI has a bug), they could send duration=999999 and it would be accepted silently.

Suggested fix:

if req.Duration < -1 || req.Duration > 3600 {
    rsp.Ret.RetCode = int(errorcode.ErrorCode_MasterParamsError)
    rsp.Ret.RetMsg = "duration must be in [-1, 3600]"
    return
}

(Note: SetTimeout doesn't need an upper bound since arbitrarily large positive timeouts are valid there, but refresh has a specific window-extension semantic where a ceiling is appropriate.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. CubeMaster Refresh() now validates duration in [-1, 3600], matching CubeAPI's RefreshRequest constraint.

Comment thread CubeAPI/src/handlers/sandboxes.rs Outdated
body.validate()
.map_err(|e| AppError::BadRequest(e.to_string()))?;

let duration = body.duration.unwrap_or(0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question (non-blocking): The default for an omitted duration is 0, which in this PR's semantics means "immediate timeout":

let duration = body.duration.unwrap_or(0);

A caller who forgets to set duration will have their sandbox immediately scheduled for expiry rather than getting a refreshed lease. I understand this likely matches E2B's behavior, but could we either:

  1. Add an explicit doc comment on RefreshRequest warning that omitting duration equals duration=0 (immediate timeout), or
  2. Consider whether None should mean "just rebase the idle clock without changing the timeout" instead?

Not blocking — just raising it since it's a sharp edge for API consumers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Documented. I kept the existing behavior for compatibility, but added an explicit RefreshRequest.duration comment noting that omitted duration is treated as 0, which means immediate timeout, and -1 means never-timeout.

return
}

func publishUpdateTimeout(ctx context.Context, req *types.UpdateRequest) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: This function is one of the two core additions in this PR but has no doc comment. A brief explanation would help future maintainers:

// publishUpdateTimeout syncs the requested timeout to the lifecycle
// metadata channel after a successful sandbox resume. It is a
// best-effort operation: failures are logged but do not affect the
// resume response.
//
// Only acts on "resume" action with a non-nil Timeout. Pause
// requests and resumes without an explicit Timeout are silently
// skipped (preserving the existing timeout without rebasing the
// idle clock).
func publishUpdateTimeout(ctx context.Context, req *types.UpdateRequest) {

This makes it immediately clear that: (1) it's resume-only, (2) failures are non-fatal, and (3) nil Timeout means "don't touch the timeout."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed as suggested above.

SandboxID string `json:"sandbox_id" p:"sandbox_id" v:"required"`
InstanceType string `json:"instance_type" p:"instance_type" v:"required"`
Action string `json:"action" p:"action" v:"required"`
Timeout *int `json:"timeout,omitempty" p:"timeout"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: The new Timeout field would benefit from a comment explaining its semantics, similar to how the SetTimeoutRequest type has detailed field documentation:

// Timeout is the new idle TTL in seconds for "resume" actions.
// Accepted values: -1 (never timeout), 0 (immediate timeout),
// or a positive number of seconds. nil preserves the existing
// timeout without rebasing the idle clock.
Timeout *int `json:"timeout,omitempty" p:"timeout"`

The nil vs 0 vs -1 distinction is subtle enough that future readers will appreciate the hint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed as suggested above.

@fslongjin fslongjin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello, can you add some e2e tests under tests/e2e/sdk_compat ?

@chenhengqi

Copy link
Copy Markdown
Collaborator

@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch 2 times, most recently from 534819b to d51bc68 Compare July 22, 2026 15:20
// requests and resumes without an explicit Timeout are silently
// skipped (preserving the existing timeout without rebasing the
// idle clock).
func publishUpdateTimeout(ctx context.Context, req *types.UpdateRequest) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: publishUpdateTimeout discards the refreshTimeoutMeta return value, masking provider failures

refreshTimeoutMeta is called here for its side effect on the provider — the returned endAt is intentionally discarded. However, when the provider fails, the only evidence is a Warn-level log line. The HTTP response to the caller remains Success with no indication the lifecycle metadata channel may be stale.

Consider bumping the log to Error level when the provider fails during a resume, or adding a metrics counter so operators can monitor provider sync failures.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Intentionally keeping Warn: metadata publishing is best-effort by design (the function comment notes failures never affect the resume response) — a failure only stalemates the metadata channel, not correctness, so Error-level would create unnecessary alert noise.

Comment thread tests/e2e/sdk_compat/cases/lifecycle/test_pause_resume.py
Comment thread CubeAPI/src/models/mod.rs Outdated
@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from e740d2b to 78e3a5a Compare August 12, 2026 16:33
if req.Duration <= 0 {
var duration int
if req.Duration == nil {
duration, _ = resolveTimeoutSeconds(nil, config.GetConfig().CubeletConf.DefaultTimeoutInsec)

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 omitted-duration fallback resolves the cluster default via resolveTimeoutSeconds(nil, DefaultTimeoutInsec), then validates the resolved value against the refresh range. Two edge cases fall out:

  1. If default_timeout_insec is configured > 3600 (the create path, ConstructCubeletReq, accepts any positive default with no upper bound), an omitted-duration refresh returns 400 "duration must be -1 or in [1, 3600]".
  2. If default_timeout_insec is unset (0), resolveTimeoutSeconds returns NeverTimeout (-1), so an omitted-duration refresh silently flips the sandbox to never-timeout — a much stronger side effect than "extend with the platform default" — and the sandbox won't auto-pause until an explicit set_timeout restores a TTL.

Since this "omitted duration" path is new in this PR, consider clamping the resolved default into the accepted range (or bounding/documenting default_timeout_insec to [1, 3600]) so it can't 400 or silently disable timeouts depending on cluster config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both edges are addressed in the final revision: refresh no longer has a 3600 cap (validation now only requires -1 or a positive value), so a >3600 cluster default no longer 400s; the omitted-duration → cluster-default behavior is intentional and covered by a dedicated warning and behavior-table entry in both the English and Chinese docs.

Comment thread CubeMaster/pkg/service/sandbox/sandbox_update.go
const NEVER_TIMEOUT_SECONDS: i32 = -1;
const E2B_USER_AGENT_MARKERS: [&str; 3] =
["e2b-python-sdk/", "e2b-js-sdk/", "e2b-code-interpreter/"];

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 User-Agent marker check is trivially spoofable (any client can set User-Agent: e2b-js-sdk/... and receive the far-future sentinel) and, conversely, breaks for genuine E2B traffic that passes through a proxy that rewrites or strips User-Agent. This is documented in docs/guide/lifecycle.md as a deliberate tradeoff, so it's acceptable — but note the behavior will also silently diverge if an E2B SDK ever changes its User-Agent marker (the match is a case-insensitive substring on three exact markers). Adding a debug log when the compat is applied would make the divergence observable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Background: for never-timeout (timeout=-1) sandboxes, Cube-native responses omit endAt, while E2B SDK models require endAt to be a valid datetime — this PR therefore adds a compat branch that returns the 9999-12-31 sentinel only on E2B-facing paths.

The design question we think more worth discussing here is: given that E2B SDKs must receive an endAt, should Cube change its behavior uniformly (returning endAt to Cube-native callers as well), or should CubeAPI differentiate by SDK and respond differently? This PR takes the latter route — distinguishing via User-Agent so Cube-native semantics stay untouched — and the tradeoffs (spoofability, proxy rewriting) are documented in lifecycle.md. So we're not making further changes here for now.

@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from 78e3a5a to ba2ed56 Compare August 12, 2026 17:32
Comment thread CubeMaster/pkg/service/sandbox/sandbox_update.go Outdated
}
if req.Duration <= 0 {
var duration int
if req.Duration == nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Refresh with omitted duration silently makes a sandbox never-timeout on the default cluster.

When req.Duration == nil, this resolves the cluster default via resolveTimeoutSeconds(nil, DefaultTimeoutInsec). CubeMaster/conf.yaml ships default_timeout_insec: -1, and resolveTimeoutSeconds maps any default <= 0 to NeverTimeout. So a plain "keep this sandbox alive" refresh with no duration permanently disables the sandbox's idle timeout. It is consistent with create semantics and documented, but it's a sharp edge clients won't expect — worth an explicit warning in the docs/API or a guard.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This behavior is intentional (omitted duration falls back to the cluster default, consistent with create semantics and the direction the maintainer requested), and the requested docs warning is in place: both the English and Chinese lifecycle docs carry a dedicated warning box plus a behavior-table entry stating that an omitted duration becomes never-timeout under the default config, and to pass an explicit positive duration when a finite TTL is needed.

## Summary

Align refresh, resume/connect, and set_timeout timeout semantics across CubeAPI, CubeMaster, and SDK compatibility tests.

## Changes

- Reject refresh duration 0 and invalid out-of-range values while preserving omitted duration as the CubeMaster cluster default.

- Keep resume/connect timeout 0 as preserve-existing, accept -1 as never-timeout, and reject values below -1.

- Add SDK compatibility E2E coverage for refresh, set_timeout, and resume timeout values across CubeSandbox and E2B backends.

## Verification

- go test ./pkg/service/sandbox -run 'Test(SetTimeout|Refresh|Update).*Timeout|TestRefreshValidation|TestRefreshOmitted|TestRefreshTimeoutMeta' -count=1

- cargo test --locked models::tests

- pytest --run-e2e --sdk-e2e-backends=cubesandbox,e2b -q tests/e2e/sdk_compat/cases/lifecycle/test_refresh.py tests/e2e/sdk_compat/cases/lifecycle/test_timeout.py tests/e2e/sdk_compat/cases/lifecycle/test_pause_resume.py::test_resume_accepts_never_timeout tests/e2e/sdk_compat/cases/lifecycle/test_pause_resume.py::test_resume_accepts_zero_and_positive_timeout tests/e2e/sdk_compat/cases/lifecycle/test_pause_resume.py::test_resume_rejects_invalid_negative_timeout

Signed-off-by: zry <1292625211@qq.com>
Assisted-by: Codex:GPT-5
@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from ba2ed56 to ca88e40 Compare August 13, 2026 17:56
Comment thread CubeAPI/src/handlers/sandboxes.rs
Comment thread CubeMaster/pkg/service/sandbox/sandbox_timeout.go
Comment thread CubeMaster/pkg/lifecycle/init.go Outdated
@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from ca88e40 to 6427d67 Compare August 14, 2026 10:28
Comment thread CubeAPI/src/handlers/sandboxes.rs
Comment thread CubeMaster/pkg/service/sandbox/sandbox_timeout.go
}
// refreshTimeoutMeta updates lifecycle metadata through the timeout provider.
// Resume does not return endAt, so the computed value is intentionally ignored.
refreshTimeoutMeta(ctx, req.SandboxID, *req.Timeout)

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 nil/0 resume path is guarded by the atomic Lua RebaseTimeoutWindow, but this explicit-timeout path goes through refreshTimeoutMetaRefreshTimeout, which is a non-atomic LoadMetaHSET/XADD read-modify-write. The Lua script comment claims atomicity prevents resume from "writing back a stale timeout over a concurrent set_timeout or refresh request" — that guarantee only holds for the nil/0 branch. A resume carrying an explicit timeout (and any set_timeout/refresh) can still clobber a concurrent update. Not a regression, but the protection is asymmetric; routing the explicit-timeout case through a comparable atomic script (set timeout_seconds/created_at/end_at in one EVAL) would close the gap.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in exactly this direction: storeTimeoutProvider.RefreshTimeout now routes through SetTimeoutWindow's single-EVAL script, atomically replacing the timeout, starting a new window, and publishing the event (lifecycle/init.go:61-68, lifecycle/store.go:189-216) — the explicit-timeout path shares the same atomic script as the nil/0 path, so the protection is no longer asymmetric.

@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from 6427d67 to 020826e Compare August 14, 2026 10:52
Comment thread CubeMaster/pkg/service/sandbox/sandbox_update.go Outdated
Comment thread CubeAPI/src/handlers/sandboxes.rs
@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch 2 times, most recently from 5110931 to 1ea6b20 Compare August 14, 2026 13:22
Comment thread CubeAPI/src/handlers/sandboxes.rs
Comment thread CubeMaster/pkg/service/sandbox/sandbox_timeout.go
// preserves the stored timeout while moving its CreatedAt and EndAt forward
// from the resume time. Metadata updates are best effort and never change the
// resume response.
func publishUpdateTimeout(ctx context.Context, req *types.UpdateRequest) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Explicit-timeout resume reports success even when the timeout was not persisted. For a legacy sandbox with no lifecycle metadata, SetTimeoutWindow returns (0, nil) — no error, no metadata created — so refreshTimeoutMeta falls back to a computed endAt and the resume succeeds while the requested timeout never takes effect. The RebaseTimeoutWindow error path (line 115) is likewise only logged. Since the resume response carries no indication, a client can believe resume(timeout=600) was applied when it wasn't. Consider having the store distinguish "metadata missing" from "success" so this path at least logs clearly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed as suggested: the store's Lua script returns an explicit not-found marker for sandboxes without metadata, and SetTimeoutWindow/RebaseTimeoutWindow surface it as a "lifecycle metadata for sandbox %s was not found" error (lifecycle/store.go:26-28, 227-228), so this path now logs clearly instead of silently returning (0, nil). The resume response stays Success by the best-effort design (see the function comment).

@emailcannotbeblank
emailcannotbeblank force-pushed the fix/add-missing-timeout-field branch from 1ea6b20 to d407231 Compare August 14, 2026 13:52
Comment thread CubeAPI/src/handlers/sandboxes.rs
Comment thread CubeAPI/src/models/mod.rs
Comment thread CubeMaster/pkg/service/sandbox/sandbox_timeout.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

4 participants