feat(clm): add active-standby failover

[chenhengqi]

Run two warm CLM replicas in Kubernetes and use a Redis lease to gate singleton sweep and prune work while both replicas serve resume requests.

Use broadcast XREAD consumption, promotion catch-up, fencing epochs, and versioned state CAS to prevent stale replicas from overwriting newer state. Add Helm configuration, readiness observability, Redis transaction tests, and bilingual documentation. Keep one-click deployment single-replica.

[cubesandboxbot]

Review: feat(clm) — add active-standby failover (#1516)

AI-generated review. This is an automated code review; it has not been approved by a human reviewer. Findings are ranked most-severe first; each includes a concrete trigger scenario.

Summary

The PR runs two warm CLM replicas behind the chart Service and uses a Redis lease (cube:v1:shared:lock:lifecycle-manager:leader) to gate singleton work (sweep, prune, proxy-push) while both replicas serve resume requests. It adds broadcast XREAD consumption with independent per-replica cursors, promotion catch-up with a drain window, fencing generations, and versioned state CAS to keep a stale replica from overwriting newer state. One-click deployment stays single-replica (election disabled).

The overall design is sound: the WATCH/MULTI lease renew/release avoids Lua, the local deadline sits safely inside the Redis TTL, ShouldApply monotonic gating prevents double-apply between consumeStream and catchUpGeneration (both serialize on eventApplyMu), and standby fleets don't grow unboundedly because the in-memory next map drops expired heartbeats on every replica. The two items I'd address before merge are F1 (resumer availability regression) and F2 (empty-stream rebuild loop).

Findings

F1 — Resumer transport-error retention regresses single-instance availability (Medium)

cube-lifecycle-manager/internal/resumer/resumer.go:259

The new default branch in callCubeMasterResume retains the resuming state key (TTL = StateLockTTL, default 60s) whenever CubeMaster.Resume fails with a transport/timeout error instead of a structured API error. This is unconditional — it applies with leader election disabled too.

Trigger: a single dropped HTTP connection to CubeMaster during a resume. The lock is never cleared (nothing else changes the key), so every subsequent resume request for that sandbox enters AcquireResume → sees resuming → waitForRunning → blocks until its timeout, and the sandbox is effectively un-resumable for up to ~60s. Previously the lock was cleared immediately and the next request retried — and CubeMaster.Resume is idempotent (a duplicate maps to "already running" → success), so the old clear-and-retry was both safe and far more available. The PR explicitly promises to keep single-replica behavior unchanged; this violates that.

Recommendation: gate the retain-ownership branch on LeaderElectionEnabled (single-instance keeps the old behavior), and/or bound the hold below the full StateLockTTL.

F2 — CursorValid("0-0") can drive a permanent registry-rebuild loop (Medium)

cube-lifecycle-manager/internal/redisstream/stream.go:141

For a 0-0 cursor on a stream key that exists but is empty, CursorValid returns EntriesAdded <= Length. When the last entries are trimmed/deleted (Length == 0, EntriesAdded > 0) this is false forever. In consumeStream that makes Read return ErrCursorTrimmed on every poll; each iteration rebuilds the whole registry from the metadata Hash (HGETALL + rebuild) and loops: rebuild → XREAD blocks StreamReadBlock → CursorValid false → rebuild → … The cursor never advances (there is nothing to read), so the loop never terminates.

Trigger: any external trim removing the final entry (XTRIM/XDEL) — exactly the operational event the trim-recovery machinery exists for. Suggest treating an empty stream as valid for a 0-0 cursor (Length == 0 → true regardless of EntriesAdded); the Hash-snapshot rebuild is authoritative either way.

F3 — New Validate() compares a configurable value against a non-configurable one (Low–Medium)

cube-lifecycle-manager/internal/config/config.go:330

Validate now returns an error when StateLockTTL <= HTTPTimeout. HTTPTimeout is hard-coded at 10s with no env override, while StateLockTTL is set via CUBE_LCM_STATE_LOCK_TTL. Any deployment that tuned the lock TTL below 10s will fail to start after upgrade, and the error message references a value the operator cannot change without a code edit. Expose HTTPTimeout via env (and wire it into the chart), or compare against a documented constant.

F4 — eventApplyMu held across outbound HTTP pushes stalls promotion catch-up (Low)

cube-lifecycle-manager/cmd/cube-lifecycle-manager/main.go:620

consumeStream holds eventApplyMu across handleEvent, which on the leader performs fleet-wide HTTP pushes with an up-to-HTTPTimeout per-proxy budget; catchUpGeneration takes the same mutex for its whole pass. A slow CubeProxy therefore delays the promotion catch-up and markReconciled (and thus leader readiness) by as long as the push loop takes. This is a liveness coupling rather than a correctness bug — it only bites when a proxy is slow during failover — but serializing network I/O under a promotion-critical lock is worth a per-event timeout or a comment.

F5 — Promotion hydration is O(entries × proxies) with a Redis GET per entry (Low)

cube-lifecycle-manager/cmd/cube-lifecycle-manager/main.go:370

replayRegistryTo does a resolvePromotionState Redis GET per registry entry, per proxy, during promotion/fleet-join hydration — a burst of serial round-trips in a goroutine, over a registry snapshot that may be stale relative to concurrently-consumed events. It converges, but when the replica is already reconciled the state can be read from the in-memory registry (kept current by the consumer) instead of Redis.

Minor notes

• stream.Read performs an extra XRANGE (via CursorValid) on every poll, doubling Redis round-trips per read cycle; consider validating only after a gap or on a slower cadence.
• Same-state state events now re-write the Redis key and re-broadcast to the fleet on every occurrence (deliberate, tested change); this adds write/network amplification if CubeMaster emits duplicate state events.
• resolvePromotionState prefers the Redis state key over the registry's RuntimeState. On failover, a stale paused written by a failed old leader can be re-pushed to proxies even though the registry already knows the sandbox is running; the resume path self-heals it (one spurious 503), so it's a comment-worthy judgment call rather than a bug.
• The chart's validate.yaml guard (replicas ≥ 2 when election is enabled) and the anti-affinity are good footgun-preventers; worth a line in the chart README that single-node installs now run two CLM replicas by default.

Positive observations

• WATCH/MULTI lease renew/release with a local deadline ~7s inside a 10s Redis TTL gives clean stale-leader fencing without Lua.
• Versioned state CAS (v1|<streamID>|state) with legacy-value decode is a minimal, correct way to enforce monotonic state ordering.
• The promotion sequence (catch up → drain one HTTPTimeout → catch up → markReconciled → hydrate) correctly bounds the stale-writer window.
• Standby replicas consume the stream and serve resumes while only the reconciled leader sweeps/prunes/pushes.

[cubesandboxbot]

AI-generated review finding (F1, Medium). Retaining the resuming lock on a transport/timeout error regresses availability, and unconditionally so. The key is set with StateLockTTL (60s default) and, on this path, is never cleared — nothing else changes it, so AcquireResume returns resuming and every subsequent request for this sandbox goes through waitForRunning until its timeout. A single dropped HTTP connection to CubeMaster makes the sandbox un-resumable for up to ~60s.

This is a regression for single-instance deployments too (election disabled), which the PR explicitly promises to keep unchanged: the previous ClearStateNotify let the next request retry immediately, and CubeMaster.Resume is idempotent (a duplicate maps to "already running" → success), so the old clear-and-retry was both safe and more available. The cross-replica dedup rationale only applies when a second replica could actually race.

Suggest gating the retain branch on LeaderElectionEnabled and/or bounding the hold well below StateLockTTL.

[cubesandboxbot]

AI-generated review finding (F2, Medium). For a 0-0 cursor on a stream key that exists but is empty, this returns EntriesAdded <= Length, which is false forever once all entries have been trimmed/deleted (Length == 0, EntriesAdded > 0). In consumeStream, Read then returns ErrCursorTrimmed on every poll, so the loop is: rebuild registry from the Hash → XREAD blocks StreamReadBlock → CursorValid still false → rebuild again. The cursor never advances (nothing to read), so it never terminates — a silent, perpetual rebuild loop.

Trigger: any external trim that removes the last entry (XTRIM/XDEL), which is exactly the operational event the trim-recovery machinery exists for. Consider returning true for Length == 0 regardless of EntriesAdded — the Hash-snapshot rebuild is authoritative either way, and there is nothing to skip.

[cubesandboxbot]

AI-generated review finding (F3, Low–Medium). This validation compares a configurable value against a non-configurable one: HTTPTimeout is hard-coded at 10s (no env var, not in the chart), while StateLockTTL is set via CUBE_LCM_STATE_LOCK_TTL. Any existing deployment that tuned the lock TTL to ≤10s now fails to start after upgrade, and the error message points at a value the operator cannot change without a code edit. Expose HTTPTimeout via env (and wire it into the chart env block), or compare against a documented constant.

[cubesandboxbot]

AI-generated review finding (F4, Low). eventApplyMu is held across handleEvent, which on the leader issues fleet-wide HTTP pushes (meta upserts/deletes, state pushes) with an up-to-HTTPTimeout per-proxy budget. catchUpGeneration takes the same mutex for its entire catch-up pass, so a slow CubeProxy delays the promotion catch-up and markReconciled (and thus the new leader becoming ready) by as long as the push loop takes. Liveness coupling rather than a correctness bug, but worth a per-event timeout or a comment explaining why the lock deliberately spans network I/O.

[FakeLearne]

There is a potential data leak issue here during active-standby failover:

1. Delete notifications are dropped: During failover promotion catch-up and the subsequent 10s HTTPTimeout drain window, the new leader has not completed reconciliation yet, so activeLeader.IsLeader() remains false, making canWrite() evaluate to false. If an OpDelete event is consumed during this window, reg.Delete() removes the sandbox from the local in-memory registry, but push.DeleteMeta is skipped.
2. Subsequent hydration cannot compensate: When the replica is finally promoted and triggers hydrateFleet, replayRegistryTo only iterates over entries currently present in reg.Snapshot(). Since the deleted sandbox was already purged from reg in step 1, the hydrator has no tombstone record and will never push a delete request to CubeProxy.
3. Impact: Any sandbox destroyed while the old leader was crashing or during the promotion window will permanently leak in CubeProxy's 16MB cube_sandbox_meta shared dict, which can eventually exhaust shared memory or route traffic to ghost sandboxes.

Suggestion:
Deleting a metadata entry in CubeProxy is idempotent. If the replica already holds the Redis lease (lease.IsLeader() == true), we should consider allowing push.DeleteMeta to pass through immediately when consuming OpDelete. Alternatively, retain a short-lived tombstone in reg or perform a diff against CubeProxy during fleet hydration.

[FakeLearne]

A single renewal failure or transient WATCH conflict here causes immediate demotion, leading to frequent leader flapping and a prolonged "no-leader" vacuum:

1. Immediate demotion on transient glitches: hold() exits on the first err != nil (e.g. transient network timeout) or !ok (single TxFailedErr conflict in redisLeaseStore.Renew). Once hold() exits, Run() immediately invokes l.demote().
2. Prolonged dead-lock vacuum (~7s + 10s drain): Although the leader locally stepped down, the lease key in Redis was written with a 10s TTL and remains valid for ~7 more seconds. During this window, neither the old leader nor standbys can acquire the lease via SET NX. The cluster enters a 7-second leader vacuum, followed by a 10-second HTTPTimeout drain upon new promotion—resulting in ~17 seconds of interrupted background maintenance.
3. Lease deadline ignored: The lease already computes a safe local deadline (started + TTL - margin), which represents the true mathematical validity of the held lease. Demoting before this deadline arrives violates standard lease semantics (e.g. k8s leaderelection).

Suggestions:

1. Retry until local deadline expires (Primary fix): In hold(), if renew() fails due to transient network errors or TxFailedErr, do not immediately return and demote. As long as time.Now().Before(*deadline) holds true, keep the leader status active and retry renewal more frequently (e.g. every RetryInterval = 1s). Only demote when the local deadline has actually expired. Also consider adding a small retry loop (e.g. maxAttempts = 3) inside redisLeaseStore.Renew for TxFailedErr.
2. Self-reclaim on acquire (Optimization): If an instance does demote but the key in Redis still holds its own token, acquire() could atomically renew the lease and resume leadership without waiting for the full 10s HTTP drain, since no peer could have become leader in the interim.

[FakeLearne]

Retaining ownership here on unknown transport errors makes sense to prevent duplicate RPCs and protect against the sweeper pausing an in-flight resume. However, using the default StateLockTTL (60s) turns a transient timeout into a 1-minute DoS for that sandbox:

1. The 60s lock-out issue: When an ambiguous transport error occurs, leaving "resuming" with the full 60s TTL causes all subsequent client retries to enter waitForRunning in acquireResumeOwnership. Since no in-flight goroutine is actually working on the resume anymore, retries will stall until client context timeout, rendering the sandbox unresponsive for a whole minute.
2. Why we shouldn't simply clearState: Unconditionally deleting the key immediately is also dangerous: resumer errors out before updating MergeLastActive, so LastActiveMs remains stale. If the key is deleted, sweeper.go's tryPause will no longer be blocked by SETNX, and the leader's next sweep tick (within 5s) could pause the sandbox that CubeMaster just resumed or is currently resuming, causing state flapping.
3. The ambiguity window is bounded by HTTPTimeout: The server-side execution window cannot exceed HTTPTimeout (10s). The problem is not the guard itself, but the parameter choice.

Suggestion:
Instead of leaving the key with StateLockTTL (60s), downgrade the marker to a short TTL corresponding to the RPC ambiguity window upon unknown error:

default:
    if !errors.As(resumeErr, &apiErr) {
        // Retain ownership only for the ambiguity window (HTTPTimeout),
        // preventing sweeper races without locking the sandbox for 60s.
        _ = r.o.Redis.SetState(ctx, sandboxID, "resuming", r.o.HTTPTimeout)
        return errors.New("cubemaster resume result unknown: " + resumeErr.Error())
    }
    r.clearState(sandboxID)
    return errors.New("cubemaster resume: " + resumeErr.Error())

This preserves the guard against sweeper races and deduplicates in-flight RPCs, while allowing subsequent client retries to recover cleanly in ~10s instead of ~60s.

[FakeLearne]

There is a critical timing discrepancy and ambiguous error-handling flaw here that contradicts the drain window guarantees stated in the README:

1. The 1x HTTPTimeout drain window is mathematically insufficient:
   • The README states that a newly promoted leader waits one HTTPTimeout drain window so in-flight writes from the previous leader can finish.
   • However, sweeper.tryPause only checks IsLeader() once before entry (sweeper.go:180). Its internal execution chain runs on the unbounded rootCtx with multiple serial network operations:
     ProxyPush.SetState("pausing") (up to 10s) $\to$ CubeMaster.Pause (up to 10s) $\to$ rollback ProxyPush.SetState("running") (up to 10s).
   • A critical section entered right before the old leader's local demotion can easily take up to 30s before dropping its final write—landing ~16s after the new leader has already completed its 10s drain and started active maintenance.
2. Ambiguous pause errors cause ghost "running" sandboxes (traffic black hole):
   • When CubeMaster.Pause times out or encounters a transport reset, the server-side result is unknown (note that CubeMaster's sandbox lock TTL is 180s, confirming pause can be slow).
   • Treating unknown transport errors as definitive failures in default: by calling ClearStateNotify and broadcasting "running" to CubeProxy is dangerous: if the microVM was actually paused on the host, CubeProxy will route incoming traffic directly to a frozen VM without triggering auto-resume, resulting in a persistent traffic black hole.
   • Note that resumer.go:256-265 already handles this exact ambiguity correctly by retaining ownership on non-APIError transport failures.

Suggestions:

1. Align tryPause with resumer.go: On timeout or transport failure, do not immediately clear state and broadcast "running". Treat it as ambiguous to prevent misleading the proxy when a VM might actually be frozen.
2. Bound tryPause / tryKill execution: Bound each sweep decision with a timeout not exceeding a single HTTPTimeout.
3. Clarify documentation: Update the README to reflect that per-sandbox state locks (SET NX with TTL), rather than the HTTP drain, provide the actual hard fencing boundary for singleton actions.