feat(clm): add active-standby failover - #1516
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
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. SummaryThe PR runs two warm CLM replicas behind the chart Service and uses a Redis lease ( The overall design is sound: the WATCH/MULTI lease renew/release avoids Lua, the local deadline sits safely inside the Redis TTL, FindingsF1 — Resumer transport-error retention regresses single-instance availability (Medium)
The new 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 Recommendation: gate the retain-ownership branch on F2 —
|
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. Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
b4c4b96 to
e116ea9
Compare
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
| // A transport or timeout error has an unknown server-side result. | ||
| // Preserve ownership until its TTL so another replica cannot issue a | ||
| // duplicate resume while CubeMaster may already be completing this one. | ||
| if !errors.As(resumeErr, &apiErr) { |
There was a problem hiding this comment.
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.
| return true, nil | ||
| } | ||
| info, err := c.rdb.XInfoStream(ctx, lifecycle.EventStreamKey).Result() | ||
| if err != nil { |
There was a problem hiding this comment.
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.
| if c.HTTPTimeout <= 0 { | ||
| return errors.New("http timeout must be > 0") | ||
| } | ||
| if c.StateLockTTL <= c.HTTPTimeout { |
There was a problem hiding this comment.
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.
| // rebuildRegistryAfterTrim reloads the Hash snapshot after MAXLEN has dropped | ||
| // events. LatestID is captured before HGETALL so the consumer does not skip | ||
| // events CubeMaster wrote between the two reads (Hash then Stream, not a | ||
| // transaction). Local LastActiveMs / RuntimeState are preserved. |
There was a problem hiding this comment.
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.
1 participant
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.