feat(templates): E2B-style display names with CubeMaster name cache#607
feat(templates): E2B-style display names with CubeMaster name cache#607HuChundong wants to merge 1 commit into
Conversation
d9adc05 to
ff87c16
Compare
|
You can check if PR #612 meets your requirements? The scale of the changes is much smaller |
ff87c16 to
2ed302c
Compare
|
|
||
| match self | ||
| .fetch_resolved_template(&template_ref, &template_id) | ||
| .await |
There was a problem hiding this comment.
Redundant second fetch_resolved_template call
update_template_name already calls fetch_resolved_template on line 144 to get the existing template metadata (including the old display_name for cache invalidation). After the successful update, it calls fetch_resolved_template again on line 164 solely to build the response. This adds an unnecessary cross-service HTTP roundtrip + DB query.
The response could be constructed from the already-fetched existing record (which was returned just 20 lines earlier), substituting the new name for display_name. Consider eliminating this second fetch to reduce latency — especially since the CubeMaster PATCH update already returned a success envelope, so the template definition is expected to still exist.
| .await?; | ||
| let existing = self | ||
| .fetch_resolved_template(&template_ref, &template_id) | ||
| .await?; |
There was a problem hiding this comment.
Extra fetch_resolved_template purely for display_name cache invalidation
delete_template calls fetch_resolved_template solely to retrieve the old display_name for cache invalidation (line 228: for display_name in template_names(&existing.display_name)). This triggers an HTTP roundtrip to CubeMaster + a DB query just to get a single string field that's already available in CubeMaster's response to the lookup endpoint.
Consider having GET /cube/template/lookup also return display_name alongside template_id, so the single lookup call serves both the ID resolution and the cache invalidation data.
| return normalized, nil | ||
| } | ||
|
|
||
| func mapDefinitionCreateDuplicateError(err error, displayName string) error { |
There was a problem hiding this comment.
Raw database errors propagated to API responses
mapDefinitionCreateDuplicateError returns err unmodified when the error message does not contain "1062" or "Duplicate entry". This means transient MySQL errors (e.g. deadlocks, connection timeouts, schema mismatches) flow through to the API response's RetMsg field verbatim (via handleTemplateLookupAction line 118 and handleTemplateDisplayNameAction line 168).
While these are authenticated endpoints, exposing raw DB error texts can reveal internal schema details, engine versions, and connection strings. Consider wrapping non-duplicate errors into a generic internal-error message before returning to clients, while logging the raw error server-side.
| error::{AppError, AppResult}, | ||
| }; | ||
|
|
||
| const CACHE_TTL: Duration = Duration::from_secs(60); |
There was a problem hiding this comment.
Cache TTL of 60 seconds is unnecessarily short
Template display names are effectively immutable after creation — they only change through the explicit update_template_name endpoint, which triggers targeted cache invalidation. Every 60s cache miss forces a cross-service HTTP call to CubeMaster's /cube/template/lookup endpoint (which queries the DB).
Since inline invalidation already handles renames on the originating replica, a TTL of 300-600s would dramatically reduce CubeMaster load without increasing the staleness window beyond what the existing invalidation already provides. Multi-replica deployments already tolerate up to one TTL window of staleness; 60s is unnecessarily tight for a mapping that changes at human timescales.
| // | ||
|
|
||
| //! Template name → templateID resolution with an in-memory cache. | ||
| //! |
There was a problem hiding this comment.
Module doc comment omits several routes using name resolution
The doc comment lists POST /sandboxes, GET/PATCH/DELETE /templates/{templateID}, and compat-adopt as the only routes with name resolution. However, the TemplateService also resolves names for POST /templates/:templateID (rebuild), POST /templates/:templateID/builds/:buildID (start build), and GET /templates/:templateID/builds/:buildID/status (get build status). A developer reading this comment would miss roughly half the affected endpoints. Recommend updating the comment to reflect the full set of routes.
These two PRs overlap slightly, but they have completely different priorities. This PR focuses on syncing the E2B API, making template-based sandbox creation the core feature. Displaying names on the WebUI serves little purpose if those names can't actually be used to spin up sandboxes. |
2ed302c to
214dc12
Compare
214dc12 to
f801c11
Compare
19ceabf to
b297013
Compare
HuChundong
left a comment
There was a problem hiding this comment.
Automated review of b297013 — see inline notes for actionable items.
| }); | ||
| } | ||
| let key = reference.to_ascii_lowercase(); | ||
| if let Some(template_id) = self.cache_get(&key).await { |
There was a problem hiding this comment.
Multi-replica stale cache on destructive paths
get_template_build_status re-validates cache hits via fetch_resolved_template, but create_sandbox, delete_template, and update_template_name trust cached name→ID mappings for up to 300s on replicas that did not handle the rename/delete.
After a name is freed or reassigned, other CubeAPI replicas can still route POST /sandboxes or DELETE/PATCH to the old template. Consider reusing the build-status revalidation pattern (or a lightweight lookup) on cache hits for mutating endpoints.
| .template_names | ||
| .resolve_template_ref(&template_ref) | ||
| .await?; | ||
| let names_for_invalidation = if TemplateNameCache::needs_resolution(&template_ref) { |
There was a problem hiding this comment.
Service-layer tests missing for name-resolution orchestration
template_names.rs has solid unit tests, but update_template_name, delete_template (name vs tpl-id branches), and sandbox create-by-name paths in this module are untested.
A few focused tests with a mock CubeMasterClient would guard cache invalidation and the delete-by-name skip-fetch behavior added in this PR.
|
|
||
| CALL cubemaster_assert_table_exists('t_cube_template_definition'); | ||
|
|
||
| -- Drop invalid reserved-prefix names on templates (snapshots keep their names). |
There was a problem hiding this comment.
Migration preflight clears legacy names irreversibly
Lines 14–37 blank out reserved-prefix names and duplicate display names before adding the unique index. This is reasonable for greenfield but can surprise production upgrades.
Please confirm staging/release notes call this out explicitly (PR body mentions it — worth a pre-migration audit query for affected rows).
Code review (b297013)Overall the PR is in good shape after the review fixes: name validation is centralized in CubeMaster, internal DB errors are masked, redundant fetches were trimmed, and test coverage on the Master side is solid. Worth addressing / confirming
Look good
No blocking issues found for merge after acknowledging multi-replica cache behavior in release notes or follow-up. |
9438441 to
4483cba
Compare
| async fn lookup_and_cache(&self, reference: &str, key: &str) -> AppResult<String> { | ||
| let lock = { | ||
| let mut locks = self.lookup_locks.write().await; | ||
| locks | ||
| .entry(key.to_string()) | ||
| .or_insert_with(|| Arc::new(Mutex::new(()))) | ||
| .clone() | ||
| }; |
There was a problem hiding this comment.
Potential memory leak: lookup_locks entries are never removed. Every unique name ever resolved adds an Arc<Mutex<()>> to the lookup_locks map, but no code cleans them up — not when cache entries are evicted (TTL or capacity), not on invalidation. Over a long-running process with many distinct template names, this grows unbounded, violating the MAX_CACHE_ENTRIES contract on self.entries.
Suggestion: Either clean up lookup_locks entries alongside cache eviction (cache_set / invalidate_name), or switch to a fixed-size striped lock pool (e.g., 64 or 256 slots via usize::from_str_bytes hash) to bound memory and reduce write-lock contention on the lookup_locks RwLock.
| /// Resolve a display name for mutating API paths, always refreshing via lookup. | ||
| pub async fn resolve_template_ref_for_mutation(&self, reference: &str) -> AppResult<String> { | ||
| let reference = reference.trim(); | ||
| if reference.is_empty() { | ||
| return Err(AppError::BadRequest( | ||
| "template reference is empty".to_string(), | ||
| )); | ||
| } | ||
| if !Self::needs_resolution(reference) { | ||
| return Ok(reference.to_string()); | ||
| } | ||
| let key = reference.to_ascii_lowercase(); | ||
| self.lookup_and_cache(reference, &key).await | ||
| } |
There was a problem hiding this comment.
Doc/behavior mismatch: The doc comment says "always refreshing via lookup," but the implementation calls lookup_and_cache, which checks the cache first and returns a cached value if still valid (300s TTL). Mutating paths (delete, rebuild, adopt-baseline) can therefore operate on stale template_id mappings for up to 300s after a name is reassigned on another replica.
Suggestion: Either bypass the cache for mutation paths (clear entry before lookup), or update the doc comment to accurately describe the caching behavior (e.g., "uses cached resolution when available; cache TTL bounds cross-replica staleness").
| }}, | ||
| } | ||
| } | ||
| normalized, _ := templatecenter.NormalizeTemplateDisplayName(name) |
There was a problem hiding this comment.
Error silently discarded: NormalizeTemplateDisplayName can return an error (e.g., name exceeds 256 chars after lookup succeeded, or contains unexpected characters), but the error is discarded via _. The response will have a valid template_id but an empty/incorrect display_name, which is misleading for API consumers.
Suggestion: Log the error or fall back to the original name:
normalized := name
if n, err := templatecenter.NormalizeTemplateDisplayName(name); err == nil {
normalized = n
} else {
log.G(ctx).Warnf("lookup returned valid template but name normalization failed: %v", err)
}
PR #607 Review: E2B-style display names with CubeMaster name cacheThis is a well-structured, layered feature addition. The architecture of thin CubeAPI client → CubeMaster-owned cache → DB unique index is clean, and the cache design with singleflight coalescing and per-name write locks is thoughtfully done. The following findings are the ones I consider most noteworthy after reviewing all five dimensions. Authorization gap on template mutation (Medium severity)None of the new endpoints ( Relevant: openapi.yml contradiction: POST /templates claims 409 but never returns oneThe POST openapi.yml: "ambiguous" mapped to wrong status codeThe 400 response descriptions on Missing error code constants in RustThe magic integers Redundant
|
| async fn lookup_and_cache(&self, reference: &str, key: &str) -> AppResult<String> { | ||
| let lock = { | ||
| let mut locks = self.lookup_locks.write().await; | ||
| locks | ||
| .entry(key.to_string()) | ||
| .or_insert_with(|| Arc::new(Mutex::new(()))) | ||
| .clone() | ||
| }; |
There was a problem hiding this comment.
Potential memory leak: lookup_locks entries are never removed. Every unique name ever resolved adds an Arc<Mutex<()>> to the lookup_locks map, but no code cleans them up — not when cache entries are evicted (TTL or capacity), not on invalidation. Over a long-running process with many distinct template names, this grows unbounded, violating the MAX_CACHE_ENTRIES contract on self.entries.
Suggestion: Either clean up lookup_locks entries alongside cache eviction (cache_set / invalidate_name), or switch to a fixed-size striped lock pool (e.g., 64 or 256 slots via usize::from_str_bytes hash) to bound memory and reduce write-lock contention on the lookup_locks RwLock.
| /// Resolve a display name for mutating API paths, always refreshing via lookup. | ||
| pub async fn resolve_template_ref_for_mutation(&self, reference: &str) -> AppResult<String> { | ||
| let reference = reference.trim(); | ||
| if reference.is_empty() { | ||
| return Err(AppError::BadRequest( | ||
| "template reference is empty".to_string(), | ||
| )); | ||
| } | ||
| if !Self::needs_resolution(reference) { | ||
| return Ok(reference.to_string()); | ||
| } | ||
| let key = reference.to_ascii_lowercase(); | ||
| self.lookup_and_cache(reference, &key).await | ||
| } |
There was a problem hiding this comment.
Doc/behavior mismatch: The doc comment says "always refreshing via lookup," but the implementation calls lookup_and_cache, which checks the cache first and returns a cached value if still valid (300s TTL). Mutating paths (delete, rebuild, adopt-baseline) can therefore operate on stale template_id mappings for up to 300s after a name is reassigned on another replica.
Suggestion: Either bypass the cache for mutation paths (clear entry before lookup), or update the doc comment to accurately describe the caching behavior (e.g., "uses cached resolution when available; cache TTL bounds cross-replica staleness").
| }}, | ||
| } | ||
| } | ||
| normalized, _ := templatecenter.NormalizeTemplateDisplayName(name) |
There was a problem hiding this comment.
Error silently discarded: NormalizeTemplateDisplayName can return an error (e.g., name exceeds 256 chars after lookup succeeded, or contains unexpected characters), but the error is discarded via _. The response will have a valid template_id but an empty/incorrect display_name, which is misleading for API consumers.
Suggestion: Log the error or fall back to the original name:
normalized := name
if n, err := templatecenter.NormalizeTemplateDisplayName(name); err == nil {
normalized = n
} else {
log.G(ctx).Warnf("lookup returned valid template but name normalization failed: %v", err)
}4483cba to
52079e1
Compare
| CubeMasterError::Http(e) if e.is_timeout() || e.is_connect() => { | ||
| AppError::ServiceUnavailable(format!("CubeMaster unavailable: {e}")) | ||
| } | ||
| CubeMasterError::Http(e) => AppError::BadGateway(format!("CubeMaster request failed: {e}")), |
There was a problem hiding this comment.
Observation: When CubeMaster returns an unexpected transport error, map_resolve_err formats messages like "CubeMaster unavailable: {e}"/"CubeMaster request failed: {e}" where {e} is the full reqwest::Error Display string. This can include the internal CubeMaster URL (e.g., http://cubemaster.internal:8080/...) and connection details, which are forwarded to the API caller as 502/503 responses.
Suggestion: Log the full error server-side and return a sanitized message like "upstream service unavailable" to avoid leaking internal network topology.
|
|
||
| // ReleaseTemplateDisplayNameAfterBuildFailure clears display_name on a template | ||
| // whose image build failed so the name can be reused on a later create. | ||
| func ReleaseTemplateDisplayNameAfterBuildFailure(ctx context.Context, templateID string) { |
There was a problem hiding this comment.
Observation: ReleaseTemplateDisplayNameAfterBuildFailure is called on every build failure to release the display name so it can be reused (from failTemplateImageJob in job_repo.go line 150). If this function breaks, failed templates permanently lock their display names, preventing users from retrying under the same name.
This function has no unit tests despite non-trivial logic: it skips on empty templateID, tolerates ErrTemplateNotFound, and logs on other errors. None of these paths are exercised.
Suggestion: Add a unit test covering the success path, the empty-ID skip, and the ErrTemplateNotFound tolerance.
| } | ||
| normalized, err := NormalizeTemplateDisplayName(req.DisplayName) | ||
| if err != nil { | ||
| log.G(context.Background()).Warnf("template image job display_name normalize failed: %v", err) |
There was a problem hiding this comment.
Observation: displayNameFromTemplateImageJob (line 382) takes no context.Context parameter and uses context.Background() for logging (lines 388, 393). This severs distributed trace correlation — any log line emitted here has no link to the originating request.
Every other public function in this file accepts a context.Context. Adding one here and plumbing it from callers (job_repo.go line 157, image_job_runner.go lines 211-214) would be a small change with significant observability benefit.
Suggestion: Add ctx context.Context parameter and propagate from callers.
|
|
||
| /// Body for PATCH /templates/:templateID (update display name). | ||
| #[derive(Debug, Deserialize, ToSchema)] | ||
| pub struct UpdateTemplateRequest { |
There was a problem hiding this comment.
Observation: UpdateTemplateRequest.name is a bare String with no validation. Other request structs in this file (e.g. NewSandbox) use #[derive(Validate)] with constraints. The documented constraints (max 256 chars, ASCII letters/digits/hyphen/underscore only, no tpl-/snap- prefix) are enforced only by CubeMaster, not at the CubeAPI boundary.
This is a defense-in-depth gap: invalid names pass through CubeAPI to CubeMaster before being rejected, costing a network round-trip. A CubeMaster regression in error code mapping could produce unpredictable HTTP status codes instead of the documented 400.
Suggestion: Add #[derive(Validate)] with length and regex constraints matching the documented rules, or add a manual validator. The #[validate] derive on CreateTemplateRequest (line 601) is also never called — consider fixing both.
| CubeMasterError::Http(e) if e.is_timeout() || e.is_connect() => { | ||
| AppError::ServiceUnavailable(format!("CubeMaster unavailable: {e}")) | ||
| } | ||
| CubeMasterError::Http(e) => AppError::BadGateway(format!("CubeMaster request failed: {e}")), |
There was a problem hiding this comment.
Observation: When CubeMaster returns an unexpected transport error, map_resolve_err formats messages like "CubeMaster unavailable: {e}"/"CubeMaster request failed: {e}" where {e} is the full reqwest::Error Display string. This can include the internal CubeMaster URL (e.g., http://cubemaster.internal:8080/...) and connection details, which are forwarded to the API caller as 502/503 responses.
Suggestion: Log the full error server-side and return a sanitized message like "upstream service unavailable" to avoid leaking internal network topology.
|
|
||
| // ReleaseTemplateDisplayNameAfterBuildFailure clears display_name on a template | ||
| // whose image build failed so the name can be reused on a later create. | ||
| func ReleaseTemplateDisplayNameAfterBuildFailure(ctx context.Context, templateID string) { |
There was a problem hiding this comment.
Observation: ReleaseTemplateDisplayNameAfterBuildFailure is called on build failure to release the display name so it can be reused (called from failTemplateImageJob in job_repo.go line 150). If this function breaks, failed templates permanently lock their display names, and users cannot retry under the same name.
This function has no unit tests despite non-trivial logic: it skips on empty templateID, tolerates ErrTemplateNotFound, and logs on other errors. None of these paths are exercised.
Suggestion: Add a unit test covering the success path, the empty-ID skip, and the ErrTemplateNotFound tolerance.
6983aa7 to
25a5804
Compare
9764b1a to
91bd0af
Compare
|
|
||
| /// Validate an optional or required E2B-style template display name at the CubeAPI | ||
| /// boundary (mirrors CubeMaster NormalizeTemplateDisplayName rules). | ||
| fn validate_template_display_name(name: &str, required: bool) -> AppResult<String> { |
There was a problem hiding this comment.
Issue: Duplicate validation logic — may diverge from CubeMaster
This function mirrors NormalizeTemplateDisplayName in Go (template_display_name.go:2979) but uses Rust's is_ascii_alphanumeric vs Go's manual ASCII range checks. If the two versions diverge, a name accepted by CubeAPI will be rejected by CubeMaster (producing a confusing 500 for the client).
Recommend either making CubeAPI a thin pass-through (delegate validation to CubeMaster) or sharing generated code.
| return nil | ||
| } | ||
|
|
||
| func failTemplateImageJob(ctx context.Context, jobID, templateID string, values map[string]any) { |
There was a problem hiding this comment.
Minor: Unnecessary DB write on build failure when no display name set
ReleaseTemplateDisplayNameAfterBuildFailure fires for every build that reaches replica-creation with template_status: StatusFailed. When no display name was ever set, this executes UPDATE ... SET display_name = '' on an already-empty column. Consider short-circuiting when the display name is empty.
Note: the early failure paths (pulling, unpacking, distributing) at lines 37, 48, 78, 103, 146 don't include template_status: StatusFailed, so they don't trigger this — correct behavior.
| templateDisplayNameCache.Set(key, templateID, templateDisplayNameCacheTTL) | ||
| } | ||
|
|
||
| func evictOneDisplayNameCacheEntry() { |
There was a problem hiding this comment.
Issue: Non-deterministic eviction — O(n) on every cache miss when at capacity
templateDisplayNameCache.Items() iterates the internal map in non-deterministic Go map order. On every cache miss when the cache is at capacity, this loop may repeatedly evict and re-evict the same keys. With 4096 entries, this degrades from O(1) to O(n).
Consider maintaining a simple FIFO ring buffer of keys alongside the map, or using go-cache's own TTL eviction and removing the manual cap check.
| pub name: String, | ||
| } | ||
|
|
||
| fn validate_update_template_name(name: &str) -> Result<(), validator::ValidationError> { |
There was a problem hiding this comment.
Issue: Third copy of the same validation logic
This is the third copy of display name validation (also in templates.rs:113 and Go's NormalizeTemplateDisplayName). If Rust's is_ascii_alphanumeric and Go's manual ASCII range checks diverge, names accepted here will be rejected by CubeMaster (500 error for the client). Recommend consolidating: only the CubeMaster side should be authoritative; the Rust side should pass through or share generated code.
91bd0af to
b9fc856
Compare
|
Thanks for the PR! Sorry for the wait, please rebase |
Add cluster-wide template display names (CubeMaster display_name) so sandboxes and APIs can reference templates by human-readable name. CubeMaster owns lookup, reservation at definition creation, bounded positive/negative caches with invalidation, and a DB migration with dedupe; CubeAPI exposes thin name resolution and GET /templates/lookup; Web adds create hints, list/detail rename, and i18n. Rebased onto upstream/master (lifecycle/env_vars merge in sandboxes). Review fixes: fresh lookup on mutating paths, UpdateTemplateRequest validation, service mock tests, display-name lock cleanup on eviction. Assisted-by: Cursor:Composer Signed-off-by: carmake <gycm520@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
b9fc856 to
338f955
Compare
| fn map_resolve_err(reference: &str) -> impl FnOnce(CubeMasterError) -> AppError { | ||
| let reference = reference.to_string(); | ||
| move |err| match err { | ||
| CubeMasterError::Api { ret_code, .. } if ret_code == 130404 => { |
There was a problem hiding this comment.
Magic error codes should be named constants
Hard-coding 130404, 130409, 130400 ties the Rust client to Go-side error codes. If CubeMaster ever changes these, the Rust side silently breaks. Define named constants in the cubemaster module (e.g., const CM_NOT_FOUND: i32 = 130404) to document the intent and isolate against changes. Same applies to the map_err function further down.
| return Err(AppError::BadRequest("name is required".to_string())); | ||
| } | ||
|
|
||
| let existing = self |
There was a problem hiding this comment.
Unnecessary sequential round trip
update_template_name fetches the existing template detail via fetch_resolved_template (line 242) solely to build the response, then separately calls update_template_display_name (line 246). This adds one full HTTP round trip to CubeMaster that could be eliminated. Consider having the update endpoint on CubeMaster return the updated definition fields, or running the fetch concurrently with the update.
| return def.DisplayName | ||
| } | ||
|
|
||
| func assertDisplayNameAvailableForCreate(ctx context.Context, normalized, templateID string) error { |
There was a problem hiding this comment.
Lock contract is enforcement-free
The comment says "Caller (withDisplayNameCreateLock) already holds the per-name write lock," but Go has no way to enforce this at compile time. If this function is called without the lock, clearDefinitionDisplayNameLocked may deadlock or corrupt state. Consider adding a sync.Locker parameter so callers must explicitly pass the lock, or inline this into withDisplayNameCreateLock since it only has one caller.
| return strings.ToLower(strings.TrimSpace(name)) | ||
| } | ||
|
|
||
| func getCachedTemplateIDByDisplayName(key string) (string, bool) { |
There was a problem hiding this comment.
getCachedTemplateIDByDisplayName checks not-found before positive — reversed priority?
This function checks templateDisplayNameNotFoundCache first (line 236), then the positive cache. If a name is both in the not-found cache (stale) and the positive cache, the not-found wins and the caller gets ErrTemplateNameNotFound despite a positive entry existing. setTemplateDisplayNameCache (line 280) correctly clears the not-found cache when setting a positive entry, so stale not-found entries shouldn't persist — but if the FIFO fallback evicts a positive entry first while a not-found entry remains, an active name could be falsely reported as "not found" until the not-found TTL (30s) expires.
|
Could you take a look at #749? That change seems a lot cleaner |
Summary
name(CubeMasterdisplay_name) with cluster-wide case-insensitive uniqueness (20260623051112_template_display_name_index.sql).nameon create.POST /sandboxesand template paths accept display name ortpl-*id; CubeAPI resolves name→id once at entry, then uses existing tpl-id paths.GET /cube/template/lookup,POST /cube/template/display-name; sync name reservation at submit (ReserveTemplateDisplayNameForCreate→ 409); release name after failed builds.display_name→template_idcached intemplatecenter(300s TTL, 4096 entries, singleflight); invalidated on rename/delete/create. CubeAPI is a thin client with no local cache.Architecture
display_name, lookup cache, and invalidationCompared to E2B infra: E2B uses team-scoped aliases reserved in
RegisterBuild; we use globaldisplay_name_key+ sync submit check. We release names on build failure (E2B keeps aliases until template delete).Test plan
go test ./pkg/templatecenter/...go test ./pkg/service/httpservice/cube/...cargo test -p cube-apigo test ./pkg/base/dao/migrate/ -run TestMigrationFilenames20260623051112_template_display_name_indexon staging (run audit queries in migration header first)name→ create sandbox by name when READYMigration note
This migration may clear legacy
tpl-/snap-prefixed names and dedupe display names before adding the unique index. Run preflight audit queries on staging before production.Assisted-by: Cursor:Composer