fix(cubemaster): recover stranded pause snapshots

[lkml-likexu]

Problem

A pause can complete on Cubelet while its caller times out, or Master can exit before recording the result. The leftover READY, timeout-FAILED, or CREATING binding makes the next auto-pause return generic 130400. CLM then rolls CubeProxy back to running although the backend is PAUSED, so the next request bypasses auto-resume and ends in HTTP 504.

Fix

• Probe Cubelet only when a leftover binding exists; fresh pauses still issue no cubelet.List call.
• Retry once when the first live-state probe fails or returns nil; each List has the existing 30-second bound and the pause lock TTL is 180 seconds.
• Return TaskStateInvalid only when the live probe confirms PAUSED:
  • READY converges directly.
  • FAILED converges only for an RPC-timeout failure and is healed to READY first.
  • CREATING is stale once the retry holds the sandbox lock and is healed to READY first.
• Keep RUNNING, successful-but-not-found probes, repeated probe failures, and explicit failures as hard errors.
• Prefer the binding's node placement while healing and log initial binding lookup failures for diagnosis.
• Preserve the prior any-PAUSED container scan semantics.
• Emit [cube:already-paused] only on this idempotent branch; CubeAPI requires both 130490 and this marker before mapping redundant pause to HTTP success.

Tests

• Classifier coverage for READY/FAILED/CREATING across PAUSED, RUNNING, and inconclusive states, including nil-record safety.
• Pause-level coverage that CREATING and timeout-FAILED convergence call Complete before returning the marker.
• CubeAPI positive and negative contract tests for the stable marker.
• Targeted CubeMaster tests passed 20 consecutive runs:
  • go test -short -count=20 -run TestPauseSandbox\\|TestShouldTreatAsAlreadyPaused\\|TestRecoverTimedOutPauseForResume ./pkg/service/sandbox
  • go test -short ./pkg/pausesnap

Assisted-by: Claude Code:claude-opus-4.7
Signed-off-by: Like Xu likexu@tencent.com

[cubesandboxbot]

Review: fix(cubemaster): treat existing pause snapshot as idempotent already-paused (PR #1370)

AI-generated review — no human approval claimed.

Overview

This PR fixes a stranded-sandbox bug: when a pause RPC times out on the caller (CLM) but actually completes on Master/Cubelet, a pause-snapshot binding is left behind. The next auto-pause hit pausesnap.Begin, which returned a generic MasterParamsError (130400). CLM only treats TaskStateInvalid (130490) as "already paused", so it rolled the CubeProxy state back to "running" — while the backend was genuinely paused — leaving auto-resume unable to fire and surfacing to the SDK as HTTP 504.

The fix is split into three coordinated parts:

1. CubeMaster pausesnap.Begin now wraps a new sentinel ErrAlreadyExists only for READY bindings (the idempotent already-paused signal); CREATING/FAILED bindings keep a plain descriptive error.
2. CubeMaster pauseSandbox probes the live Cubelet state when a leftover binding exists, and — via the pure classifier shouldTreatAsAlreadyPaused — reports TaskStateInvalid (130490) plus a stable marker only when the probe confirms PAUSED. FAILED bindings additionally require the failure to be an RPC timeout (mirroring recoverTimedOutPauseForResume). A stale binding on a genuinely RUNNING sandbox, or an inconclusive probe, stays a hard 130400 so CLM never converges a running box to paused.
3. CubeAPI pause_sandbox keys its redundant-pause → HTTP 200 decision on the exact marker string ([cube:already-paused]) rather than the bare 130490 code, so a Cubelet-originated 130490 passed through verbatim is not swallowed as success.

I traced the end-to-end mechanism: Begin's ErrAlreadyExists wrap → the probe + classifier gates (READY/CREATING/FAILED × PAUSED/RUNNING/inconclusive) → the marker-carrying 130490 return → parse_response surfacing it as CubeMasterError::Api → the new match arm → Ok(()). The classifier is logically correct, the safety gates (never converge a running box) are properly placed, code values are pinned (130490/130400), and a clean pause still issues zero cubelet.List calls. Both sides of the marker contract are pinned by tests. The core change is sound.

Findings

1. [Medium — residual risk] The fix's success path depends on a single, non-retried live probe; a transient probe failure re-triggers the exact 504 symptom

shouldTreatAsAlreadyPaused requires liveFound && liveState == CONTAINER_PAUSED, and probePauseLiveState returns (UNKNOWN, false) on any cubelet.List error or nil response. So on a redundant pause of an already-paused sandbox, a transient probe failure (network blip, Cubelet restart, brief RPC timeout) makes the classifier return false → pauseSandbox returns 130400 → CubeAPI 400 → CLM rolls the proxy back to "running" → the next request hits the paused upstream → the same HTTP 504 this PR is fixing. The safety rationale (never converge a running box — the stale-READY-on-RUNNING case is real) is legitimate and documented, but it means the fix is only as reliable as the probe call, which is exactly the kind of transient failure that caused this bug class in the first place. Consider at least one probe retry, or a short probe timeout, so a transient probe failure doesn't regress to the failure mode being fixed.

Anchored at CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go:94 (the single probePauseLiveState call shared by both consumers).

2. [Low — consistency] FAILED-timeout convergence is not healed to READY, unlike the CREATING path

The CREATING branch heals the stranded binding via pausesnap.Complete before reporting already-paused, so the binding reflects the confirmed-paused reality. The FAILED-timeout branch returns TaskStateInvalid without healing, leaving the binding FAILED on a genuinely paused sandbox. Downstream, TryDeletePaused then treats that sandbox as a failed pause (full Destroy, no tombstone annotation) instead of the tombstone path a READY binding gets, and status/GetBySandbox consumers see FAILED. It likely still works (checkDestroyRet treats not-found as success), but healing FAILED-timeout to READY here — mirroring both the CREATING heal and recoverTimedOutPauseForResume — would keep the binding consistent with reality. Note the PR makes this FAILED+PAUSED state more reachable, so the asymmetry now has a larger surface.

Anchored at CubeMaster/pkg/service/sandbox/sandbox_resume_pause.go:131.

3. [Note] Cross-language marker contract is fragile but well-guarded

The [cube:already-paused] literal is load-bearing across CubeMaster (Go) and CubeAPI (Rust), keying a 200-vs-500 decision. Both sides pin the literal in tests (already_paused_marker_is_stable in Rust; the Go tests assert alreadyPausedMarker equals the literal and that the message carries it), and the marker is implausible in pass-through Cubelet messages. This is about as good as a magic-string contract gets — just keep the CONTRACT comments honest if the string ever changes.

4. [Note — pre-existing] Probe target is existing.NodeIP, so a migrated sandbox with a leftover binding can't be converged

pauseSandbox probes existing.NodeIP (falling back to hostIP). If a sandbox was resumed on a different node with the binding left behind, the probe finds nothing → inconclusive → hard error → pause fails. This matches the pre-fix behavior of clearStalePauseBindingIfRunning and is not a regression, just a limitation of the probe target.

Test coverage

Strong. The pure classifier is table-tested directly across all branches (READY/CREATING/FAILED × PAUSED/RUNNING/inconclusive, plus nil-record), and pauseSandbox-level tests cover: READY+paused converge (asserts marker + code pins), stale-READY-running hard error, READY+probe-inconclusive hard error, generic Begin error, FAILED-timeout converge (asserts marker), FAILED-explicit hard error, and the CREATING heal (asserts Complete arguments). Store-level tests pin Begin's ErrAlreadyExists wrapping and the non-wrapping of CREATING/FAILED. The Rust side covers the marker-based 200 decision plus two negative controls (backend fault, unrelated 130490 without the marker). I did not find CI failures in the diff.

[ls-ggg]

Pause/resume is getting a major overhaul. Let's wait for it to merge, then rebase + retest

[chenhengqi]

CLM then rolls CubeProxy back to running although the backend is PAUSED

This implies that the Cubelet should be idempotent for pause opertion.